cirq_google.GridDevice, minus gateset and gate durations #5203

verult · 2022-04-06T01:01:56Z

Part of #5050

Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up.
Added a device_specification_validation module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like:
- Qubit self-loops in qubit pairs
- Qubit pairs contain qubits not in the valid qubit set.
- Gate durations contains a gate which is missing in the gateset.

__str__() and _repr_pretty_ are nearly all copied from SerializableDevice.

Part of #5050

Also fixes #5197

Question: Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general.

@dstrain115 @MichaelBroughton

MichaelBroughton

Looking really good. Just a few minor tweaks then we should be good to merge.

MichaelBroughton · 2022-04-14T20:21:53Z

cirq-core/cirq/devices/grid_device_metadata.py

+        return cls(
+            qubit_pairs,
+            gateset,
+            None if gate_durations is None else dict(gate_durations),
+            all_qubits,
+        )


Why add this None if gate durations ?

dict(None) throws TypeError: 'NoneType' object is not iterable. Alternatively we could also give gate_durations a default value: gate_durations=[]. That might be cleaner.

Non-default arguments can't be after default arguments. Will shifting the order of method parameters here break JSON backward compatibility?

MichaelBroughton · 2022-04-14T20:22:16Z

cirq-core/cirq/ops/gateset.py

+        gates_str = f'{self._gates_repr_str}, ' if len(self._gates_repr_str) > 0 else ''
        return (
            f'cirq.Gateset('
-            f'{self._gates_repr_str}, '
+            f'{gates_str}'


Why change here ?

This addresses #5197

Please add a test to gateset_test.py. Also, If the change is unrelated and is not blocking this PR, then I'd prefer if we can split it out in a separate PR.

Added and moved to #5322

MichaelBroughton · 2022-04-14T20:24:18Z

cirq-google/cirq_google/devices/google_device.py

+
+
+@cirq.value_equality
+class GoogleDevice(cirq.Device):


Might want to call it something like UniformGoogleDevice or GoogleGridDevice just to convey the message that this type identifies and works for devices that have uniform gates in all directions etc. Since we are surfacing a GridDeviceMetadata here we want to ensure that the user knows they won't get a device that doesn't have the same gates and connectivity everywhere.

Sounds good. How about just GridDevice? I'm cautious with "uniform" because for example some 2-qubit gates may be missing. The "Google" descriptor is implicit from the package name and was there only because I couldn't think of a better descriptor :)

I am in favor of GoogleGridDevice or GridDevice.

MichaelBroughton · 2022-04-14T20:25:22Z

cirq-google/cirq_google/devices/google_device.py

+        https://github.com/quantumlib/Cirq/blob/3969c2d3964cea56df33b329f036ba6810683882/cirq-google/cirq_google/api/v2/device.proto#L13
+    )
+    is the main specification for device information surfaced by the Quantum Computing Service.
+    Thus, this class is should be instantiated using a `DeviceSpecification` proto via the
+    `from_proto()` class method.


Love the verbosity here!

MichaelBroughton · 2022-04-14T20:26:05Z

cirq-google/cirq_google/devices/google_device.py

+    @classmethod
+    def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':


Why is the from_proto method preffered over init design-wise ?

Hmm good question, I don't have any good reasons other than following the existing pattern in SerializableDevice. We also don't ever use the SerializableDevice constructor anywhere else. @dstrain115 do you happen to know?

The full DeviceSpecification would have to be stored in the repr and json, which could potentially be verbose?

MichaelBroughton · 2022-04-14T20:28:35Z

cirq-google/cirq_google/devices/google_device.py

+        try:
+            metadata = cirq.GridDeviceMetadata(
+                qubit_pairs=qubit_pairs,
+                gateset=cirq.Gateset(),  # TODO(#5050) implement
+                all_qubits=all_qubits,
+            )
+        except ValueError as ve:
+            raise ValueError("DeviceSpecification is invalid.") from ve


Nit: Do we want to catch the exception here and introduce this dependency on the behavior or GridDeviceMetaData's error behavior ? By extending our API for from_proto here to accomodate errors in downstream code we are kind of bringing the error checking from downstream code upstream here to some extent. Do we want to do that in this case ?

Yeah good point. It's perhaps better to explicitly call a validation function before doing any parsing, and instead leave this exception here as a last line of defense. Better not to tightly couple with downstream errors. Will update.

verult

Thanks for the review!

verult · 2022-04-26T22:38:59Z

cirq-core/cirq/devices/grid_device_metadata.py

+        return cls(
+            qubit_pairs,
+            gateset,
+            None if gate_durations is None else dict(gate_durations),
+            all_qubits,
+        )


dict(None) throws TypeError: 'NoneType' object is not iterable. Alternatively we could also give gate_durations a default value: gate_durations=[]. That might be cleaner.

verult · 2022-04-26T22:40:24Z

cirq-core/cirq/ops/gateset.py

+        gates_str = f'{self._gates_repr_str}, ' if len(self._gates_repr_str) > 0 else ''
        return (
            f'cirq.Gateset('
-            f'{self._gates_repr_str}, '
+            f'{gates_str}'


This addresses #5197

verult · 2022-04-26T22:46:47Z

cirq-google/cirq_google/devices/google_device.py

+    @classmethod
+    def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':


Hmm good question, I don't have any good reasons other than following the existing pattern in SerializableDevice. We also don't ever use the SerializableDevice constructor anywhere else. @dstrain115 do you happen to know?

verult · 2022-04-26T22:56:13Z

cirq-google/cirq_google/devices/google_device.py

+        try:
+            metadata = cirq.GridDeviceMetadata(
+                qubit_pairs=qubit_pairs,
+                gateset=cirq.Gateset(),  # TODO(#5050) implement
+                all_qubits=all_qubits,
+            )
+        except ValueError as ve:
+            raise ValueError("DeviceSpecification is invalid.") from ve


Yeah good point. It's perhaps better to explicitly call a validation function before doing any parsing, and instead leave this exception here as a last line of defense. Better not to tightly couple with downstream errors. Will update.

verult · 2022-04-26T23:00:16Z

cirq-google/cirq_google/devices/google_device.py

+
+
+@cirq.value_equality
+class GoogleDevice(cirq.Device):


Sounds good. How about just GridDevice? I'm cautious with "uniform" because for example some 2-qubit gates may be missing. The "Google" descriptor is implicit from the package name and was there only because I couldn't think of a better descriptor :)

verult · 2022-04-27T18:45:25Z

Addressed comments, and added a validation function which is called at the beginning of from_proto, but also will eventually be called on server side prior to sending the DeviceSpecification.

dstrain115

Looking good! I like the very descriptive docstring blocks within the class.

dstrain115 · 2022-04-27T13:33:05Z

cirq-core/cirq/devices/grid_device_metadata.py

+        return cls(
+            qubit_pairs,
+            gateset,
+            None if gate_durations is None else dict(gate_durations),


Optional: Do you think dict(gate_durations) if gate_durations is not None else None is more readable?

Keeping the slightly more complicated part up front sgtm, will update

dstrain115 · 2022-04-27T14:02:47Z

cirq-google/cirq_google/devices/google_device.py

+
+
+@cirq.value_equality
+class GoogleDevice(cirq.Device):


I am in favor of GoogleGridDevice or GridDevice.

cirq-google/cirq_google/devices/grid_device.py

dstrain115 · 2022-04-28T12:23:59Z

cirq-google/cirq_google/devices/grid_device.py

+    Example use cases:
+
+        * Get an instance of a Google grid device.
+        >>> device = cirq_google.get_engine().get_processor('weber').get_device()


I wonder if we should create an example that does not require access to the API. This could be a follow-up issue/PR if this is not yet done. For instance, using the virtual engine stuff that we are building.

Sounds good. Once the virtual engine work is available I imagine we'd do a docs sweep to update all the examples that access the API. Leaving this as no-op for now.

dstrain115 · 2022-04-28T12:25:21Z

cirq-google/cirq_google/devices/grid_device.py

+        >>> print(device)
+
+        * Determine whether a circuit can be run on the device.
+        >>> device.validate_circuit(circuit)  # Raises an exception if the circuit is invalid.


nit: may be more descriptive to say which exception is raised, e.g. ValueError so that people can put try catch around it easily.

dstrain115 · 2022-04-28T12:26:10Z

cirq-google/cirq_google/devices/grid_device.py

+
+    For Google devices, the
+    [DeviceSpecification proto](
+        https://github.com/quantumlib/Cirq/blob/3969c2d3964cea56df33b329f036ba6810683882/cirq-google/cirq_google/api/v2/device.proto#L13


Change this like to point to master and not a specific commit.

dstrain115 · 2022-04-28T12:29:51Z

cirq-google/cirq_google/devices/grid_device.py

+
+        This class only supports `cirq.GridQubit`s and `cirq.NamedQubit`s. If a
+        `DeviceSpecification.valid_qubits` string is in the form `<int>_<int>`, it is parsed as a
+        GridQubit. Otherwise it is parsed as a NamedQubit.


Interesting choice. If this is a GridDevice, do you think the qubits should be restricted to GridQubits? If there a use case for named qubits. I would guess that it is likely a mistake if a non-grid qubit id happens.

Sg. This came from the SerializableDevice implementation, but will remove since we don't ever use NamedQubits.

Will also add validation for the GridQubit string format.

dstrain115 · 2022-04-28T12:33:29Z

cirq-google/cirq_google/devices/grid_device.py

+            raise ValueError(f'Qubit pair is not valid on device: {operation.qubits!r}')
+
+    def __str__(self) -> str:
+        # If all qubits are grid qubits, render an appropriate text diagram.


I am surprised that the below code for rendering a grid device doesn't exist somewhere already. Is the default insufficient because we do not assume pairwise connectivity between qubits?

I got this code from SerializableDevice, and there's also something similar for XmonDevice. This GridDevice will eventually replace both of them so I don't think there's a need to define a common function.

cirq.Device doesn't have a default, and I presume it's because yeah connectivity can be more general and would be hard to render. cc @MichaelBroughton

dstrain115 · 2022-04-28T12:35:20Z

cirq-google/cirq_google/devices/grid_device_test.py

+    return grid_qubits, spec
+
+
+def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:


Optional: consider adding a docstring for this and the below private functions for improved clarity.

tanujkhattar

Looking really good. Only concern is around proto validation for asymmetric and subset permutation targets.

cirq-google/cirq_google/devices/grid_device.py

tanujkhattar · 2022-05-03T19:49:32Z

cirq-google/cirq_google/devices/grid_device.py

+                gateset=cirq.Gateset(),  # TODO(#5050) implement
+                all_qubits=all_qubits,
+            )
+        except ValueError as ve:  # coverage: ignore


Can we get rid of the coverage: ignore here and below?

I added them because these lines are currently unreachable if everything works correctly, and only act as a last line of defense.

Can we add a test which gives a bad input that triggers this condition?

The same conditions that would trigger this exception would also cause DeviceSpecification validation to fail, so this is captured by the test test_grid_device_invalid_device_specification. If validation is buggy and this exception here is triggered, the test should get an exception but will fail because there's an error pattern mismatch.

Per @MichaelBroughton 's suggestion I added spec validation logic at the top of from_proto to make it explicit that errors that would've caused GridDeviceMetadata instantiation to fail are due to an invalid DeviceSpecification.

Will update the error match patterns to include "Invalid DeviceSpecification" to make sure that validation error messages are matched instead of the GridDeviceMetadata ones, in case error messages accidentally match up in the future between the two.

Is it even worth having the ValueError catching if unreachable?

I think so - it would protect against future bugs in validation logic. For example, when GridDeviceMetadata gets an additional constraint but validation isn't updated, it's useful to give users the hint that the error they are getting is due to the DeviceSpecification being invalid rather than a mistake they made.

I also think we should remove this check since the code path is unreachable.

We can add an assertion instead if we want to make sure we catch the inconsistencies between validation logic vs new constraints in GridDeviceMetadata but I'm not sure if putting everything under the umbrella of device specification errors is the right thing to do in this case.

cirq-google/cirq_google/devices/grid_device.py

…sary fields in DeviceSpecification proto.

…factor/device

… same GridDeviceMetadata as GridDeviceMetadata.repr

verult · 2022-05-06T00:30:12Z

Ready for final approvals!

verult

My bad, realized I never sent out my comment replies after pushing the code.

cirq-google/cirq_google/devices/grid_device.py

…ch pattern

dstrain115

LGTM, just a few minor nits.

cirq-google/cirq_google/devices/grid_device.py

dstrain115 · 2022-05-06T20:38:13Z

cirq-google/cirq_google/devices/grid_device.py

+                gateset=cirq.Gateset(),  # TODO(#5050) implement
+                all_qubits=all_qubits,
+            )
+        except ValueError as ve:  # coverage: ignore


Is it even worth having the ValueError catching if unreachable?

CirqBot · 2022-05-07T00:54:28Z

Automerge cancelled: The automerge label was removed.

@dstrain115

…5203) Part of quantumlib#5050 * Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up. * Added a `device_specification_validation` module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like: * Qubit self-loops in qubit pairs * Qubit pairs contain qubits not in the valid qubit set. * Gate durations contains a gate which is missing in the gateset. `__str__()` and `_repr_pretty_` are nearly all copied from `SerializableDevice`. Part of quantumlib#5050 Also fixes quantumlib#5197 **Question:** Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general. @dstrain115 @MichaelBroughton

@dstrain115

…5203) Part of quantumlib#5050 * Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up. * Added a `device_specification_validation` module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like: * Qubit self-loops in qubit pairs * Qubit pairs contain qubits not in the valid qubit set. * Gate durations contains a gate which is missing in the gateset. `__str__()` and `_repr_pretty_` are nearly all copied from `SerializableDevice`. Part of quantumlib#5050 Also fixes quantumlib#5197 **Question:** Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general. @dstrain115 @MichaelBroughton

verult requested review from MichaelBroughton and dstrain115 April 6, 2022 01:01

verult requested review from wcourtney, a team, vtomole and cduck as code owners April 6, 2022 01:01

CirqBot added the size: L 250< lines changed <1000 label Apr 6, 2022

verult force-pushed the cg-device-refactor/device branch 3 times, most recently from 3cd7c1b to 53880e6 Compare April 7, 2022 00:21

MichaelBroughton self-assigned this Apr 9, 2022

MichaelBroughton reviewed Apr 14, 2022

View reviewed changes

verult commented Apr 26, 2022

View reviewed changes

verult force-pushed the cg-device-refactor/device branch 3 times, most recently from b86ca7b to a14e2df Compare April 27, 2022 18:42

verult requested a review from MichaelBroughton April 27, 2022 18:44

verult force-pushed the cg-device-refactor/device branch 2 times, most recently from 2edef8f to 5c75527 Compare April 27, 2022 19:58

dstrain115 reviewed Apr 28, 2022

View reviewed changes

verult requested review from dstrain115 April 28, 2022 20:58

verult force-pushed the cg-device-refactor/device branch from c8743b6 to 9f3afb5 Compare April 29, 2022 00:17

verult mentioned this pull request Apr 29, 2022

GridDevice gateset, gate_duration, and compilation_target_gateset support #5315

Merged

verult changed the title ~~GoogleDevice, minus gateset and gate durations~~ GridDevice, minus gateset and gate durations May 2, 2022

verult force-pushed the cg-device-refactor/device branch from 9f3afb5 to 93c1c5d Compare May 2, 2022 20:40

verult changed the title ~~GridDevice, minus gateset and gate durations~~ cirq_google.GridDevice, minus gateset and gate durations May 2, 2022

verult added 2 commits May 2, 2022 23:38

GridDevice, minus gateset and gate durations

eb325eb

Addressed Michael's comments

a4c9e19

tanujkhattar assigned MichaelBroughton and unassigned MichaelBroughton May 3, 2022

tanujkhattar requested changes May 3, 2022

View reviewed changes

tanujkhattar self-assigned this May 3, 2022

verult added 2 commits May 4, 2022 01:35

Addressed Tanuj's comments; added TODO for removing currently unneces…

8d4498e

…sary fields in DeviceSpecification proto.

Merge branch 'master' of github.com:quantumlib/Cirq into cg-device-re…

13eb9fa

…factor/device

verult requested a review from tanujkhattar May 4, 2022 01:40

Add CompilationTargetGateset to GridDevice repr and json by using the…

22512c5

… same GridDeviceMetadata as GridDeviceMetadata.repr

tanujkhattar approved these changes May 6, 2022

View reviewed changes

verult commented May 6, 2022

View reviewed changes

cirq-google/cirq_google/devices/grid_device.py Show resolved Hide resolved

cirq-google/cirq_google/devices/grid_device.py Show resolved Hide resolved

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved

GridDevice tests: Add prefix in invalid DeviceSpecification error mat…

624f1fa

…ch pattern

dstrain115 approved these changes May 6, 2022

View reviewed changes

docstring and comment improvements

bb71a82

verult added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 7, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022

Merge branch 'master' into cg-device-refactor/device

1831706

verult removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels May 7, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022

CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022

tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 9, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 9, 2022

Merge branch 'master' into cg-device-refactor/device

1a0c141

CirqBot merged commit 4531a10 into quantumlib:master May 9, 2022

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels May 9, 2022

		@classmethod
		def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':

		return grid_qubits, spec


		def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:

cirq_google.GridDevice, minus gateset and gate durations #5203

cirq_google.GridDevice, minus gateset and gate durations #5203

Conversation

verult commented Apr 6, 2022 • edited Loading

MichaelBroughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Apr 27, 2022

dstrain115 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujkhattar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented May 6, 2022

verult left a comment

Choose a reason for hiding this comment

dstrain115 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CirqBot commented May 7, 2022

verult commented Apr 6, 2022 •

edited

Loading

verult Apr 28, 2022 •

edited

Loading

verult Apr 28, 2022 •

edited

Loading